[c++] Avoid redundant copies during row type resolution - #4246
fresh-borzoni merged 2 commits into
Conversation
Borrow unchanged STRING and BYTES values from the input row while preserving type conversions and validation. Add regression tests for borrowed storage, mixed conversions, nested rows, and invalid values. Fixes apache#4239
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@naivedogger LGTM overall 👍
One question for a follow-up: when no column actually needs converting, we still build a whole second row. Could we skip resolve_row_types entirely in that case, as #4239 also suggested?
resolve_row_types now returns the input row borrowed when no column needs converting, building a second row only for actual conversions or for padding short upsert/delete rows to full schema width. The lookup and prefix-lookup paths compact and resolve in one pass via the new resolve_dense_row_types, which likewise skips the rebuild for already-dense rows. Addresses the follow-up from the review of apache#4246.
|
Thanks for the review! Pushed a follow-up commit that does this. resolve_row_types now checks whether any column would actually change, i.e. an Int32 targeted at TinyInt or SmallInt, or a String targeted at Decimal, including inside nested rows. When nothing needs converting it returns the input row borrowed and no second row is built. Upsert and delete still pad short rows to full schema width, but a full-width row that needs no conversion now goes straight to the core writer. The lookup paths got the same treatment: they compact and resolve in one pass, and when the PK or prefix values are already at dense positions with nothing to convert, the input row is used as-is. Validation behavior is unchanged, and the fast paths are covered by new tests. |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@naivedogger Thank you, LGTM 👍
2 participants
Summary
Test Plan
🤖 AI-assisted changes - reviewed by human developer